-
-
Notifications
You must be signed in to change notification settings - Fork 398
Better gRPC error handling #1251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, it's a great improvement that's very much needed. I just have some doubt about the returning an rpc.UploadResponse
even when the status.Status
might contain errors.
64b9066
to
adfa4a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this considered a breaking change to the API?
If so, does the standard process for documenting it need to be done still?
https://arduino.github.io/arduino-cli/latest/CONTRIBUTING/#pull-request-checklist
🤦🏼 right, we should prepare a migration guide. I'll write down some notes later today... |
@@ -75,13 +75,13 @@ func ParseLibraryReferenceArgs(args []string) ([]*LibraryReferenceArg, error) { | |||
// ParseLibraryReferenceArgAndAdjustCase parse a command line argument that reference a | |||
// library and possibly adjust the case of the name to match a library in the index | |||
func ParseLibraryReferenceArgAndAdjustCase(instance *rpc.Instance, arg string) (*LibraryReferenceArg, error) { | |||
libRef, err := ParseLibraryReferenceArg(arg) | |||
libRef, _ := ParseLibraryReferenceArg(arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we fix this by handling this err
instead of ignoring it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably already clear, but I did this because this assignment of err
was ignored and the error
type from the declaration was incompatible with the change of lib.LibrarySearch()
's return type from error
to *status.Status
.
If it's to be ignored, I think it's best to make that explicit by using _
. But if an error return from ParseLibraryReferenceArg()
is significant in this context, then of course it's better to handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the err
was ignored also before this PR (it was overwritten without being checked). This should be fixed in another PR BTW.
The translation files must be updated, the rest is good. I'll approve as soon that's fixed. 👍 |
After trying the current solution for a bit I found two annoying problems:
PROPOSAL To address the two problems above, while rebasing this PR on master, I'd like to try a different approach:
|
Yes, I 100% agree with this new design. I think it also make it easier to handle errors internally and recover them gracefully. |
4a6bd7c
to
c146dae
Compare
Requested changes have been made. Thanks!
Co-authored-by: per1234 <[email protected]>
777b59c
to
63dd036
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all it's a great improvement but there are some small things to fix.
Co-authored-by: Silvano Cerza <[email protected]>
This is a proposal to uniform gRPC error handling.
For now only the
upload
command has been ported, as a proof-of-concept. It will be extended to all thearduino-cli
code base if approved.This PR, once fully implemented, should:
status.Status
instead of anerror
(theStatus
message is how the gRPC transports errors on RPC calls).codes
Status.details
field (for example theUploadRequiresProgrammerError
)